Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add flake8-annotations package for type linting #1061

Merged
merged 7 commits into from
Nov 5, 2021

Conversation

nthall
Copy link
Contributor

@nthall nthall commented Oct 28, 2021

- add pkg in requirements-dev.txt
- require pkg in pre-commit config
- add some easily resolved annotations in tests
- ignore some type warnings in tox.ini

Pull Request Checklist

Thank you for taking the time to improve Arrow! Before submitting your pull request, please check all appropriate boxes:

  • 🧪 Added tests for changed code.
  • 🛠️ All tests pass when run locally (run tox or make test to find out!).
  • 🧹 All linting checks pass when run locally (run tox -e lint or make lint to find out!).
  • 📚 Updated documentation for changed code.
  • ⏩ Code is up-to-date with the master branch.

If you have any questions about your code changes or any of the points above, please submit your questions along with the pull request and we will try our best to help!

Description of Changes

Closes: #856
This change adds the flake8-annotations plugin to enable linting of type annotations via pre-commit/tox. In order to enable that without breaking the build, I've added some annotation warnings to the flake8 ignore lists. I thought it would best be left to the maintainers to decide whether it would be more appropriate to address those warnings in separate commits/PRs.

The ignored warnings, and some brief discussion:

  • Globally ignored:
    • ANN101 Missing type annotation for self in method / ANN102 Missing type annotation for cls in classmethod
      • PEP 484 says, "In most cases the first argument of class and instance methods does not need to be annotated, and it is assumed to have the type of the containing class for instance methods, and a type object type corresponding to the containing class object for class methods." flake8-annotations warns on this case anyway; given the large number of changes required to "fix" this warning I thought it better here to ignore them and have a smaller commit that still advanced the state of the project. Of course if they're desired it would be cleaner to address that issue separately from whether to lint for annotations at all.
  • Ignored only in the tests dir:
    • ANN201 Missing return type annotation for public function
      • Adding such annotations to all tests would again make the commit pretty large.
    • ANN001 Missing type annotation for function argument
      • This warning appears throughout the tests, including in the pytest fixtures in conftest.py; again, seems like a change worth discussing separately from existing annotations.
    • HOWEVER, I did add a very few annotations in the test suite nonetheless, as they seemed to me not to be very intrusive.

    - add pkg in requirements-dev.txt
    - require pkg in pre-commit config
    - add some easily resolved annotations in tests
    - ignore some type warnings in tox.ini
@codecov
Copy link

codecov bot commented Oct 29, 2021

Codecov Report

Merging #1061 (25d1c66) into master (8f5b21f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1061   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines         2223      2223           
  Branches       350       350           
=========================================
  Hits          2223      2223           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f5b21f...25d1c66. Read the comment docs.

Copy link
Member

@jadchaar jadchaar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the controbution @nthall. Looks good, but it seems like CI is failing on a number of Python versions and OSes. You may need to replace list[str] with List[str] (List generic from the typings library).

tests/test_arrow.py Outdated Show resolved Hide resolved
@nthall
Copy link
Contributor Author

nthall commented Oct 30, 2021

@jadchaar thanks - sorry for that. I've pushed a fix. Frankly one reason I wanted to tackle this was to improve my familiarity with type annotations, as most of the code I get to work on right now is still on 2.7 with no plans for upgrade or mypy integration.

@nthall
Copy link
Contributor Author

nthall commented Oct 30, 2021

Ah, this is a tougher one. Down in the dependency chain for flake8-annotations is typed-ast, which has no plans to support pypy. I'm not sure what, if any, resolution would make sense here.

@jadchaar
Copy link
Member

Ah, this is a tougher one. Down in the dependency chain for flake8-annotations is typed-ast, which has no plans to support pypy. I'm not sure what, if any, resolution would make sense here.

Since we are only running flake8 via pre-commit we should be able to remove flake8-annotations from the requirements-dev.txt, which should allow pypy to build successfully again.

@nthall
Copy link
Contributor Author

nthall commented Nov 1, 2021

OK. I've updated requirements-dev.txt.

Happy Halloween!

@nthall
Copy link
Contributor Author

nthall commented Nov 5, 2021

I think this is done now? The only thing arguably left would be to rebase it into a cleaner commit - if you want me to do that, I will.

@anishnya
Copy link
Member

anishnya commented Nov 5, 2021

I think this is done now? The only thing arguably left would be to rebase it into a cleaner commit - if you want me to do that, I will.

No need for the cleaner commit. We can do a simple squash on it. The PR looks good to me, but I'm just waiting on approval from @jadchaar or @krisfremen before we pull it in.

@jadchaar
Copy link
Member

jadchaar commented Nov 5, 2021

Looks great thanks for the awesome work @nthall!

@jadchaar jadchaar merged commit 98bb910 into arrow-py:master Nov 5, 2021
@nthall nthall deleted the add-flake8-annotations branch November 8, 2021 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add flake8 checks for annotations
3 participants